Skip to content

Read php.ini from the dependency directory in the sessions extension - #1313

Open
ocean90 wants to merge 1 commit into
cloudfoundry:masterfrom
wearerequired:fix/sessions-php-ini-path
Open

Read php.ini from the dependency directory in the sessions extension#1313
ocean90 wants to merge 1 commit into
cloudfoundry:masterfrom
wearerequired:fix/sessions-php-ini-path

Conversation

@ocean90

@ocean90 ocean90 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Staging fails for any app with a bound Redis or Memcached session service:

Running extension Compile phase
**ERROR** Extension compilation failed: extension sessions compile failed: failed to load PHP config: failed to load php.ini: failed to read config file php/etc/php.ini: open php/etc/php.ini: no such file or directory
Failed to compile droplet: Failed to run all supply scripts: exit status 14

This is the failure reported in #1291.

Cause

NewPHPConfigHelper builds its paths from ctx.BuildDir:

phpIniPath: filepath.Join(ctx.BuildDir, "php", "etc", "php.ini"),

NewContext fills that field from os.Getenv("BUILD_DIR"), which the lifecycle never exports, so it is empty and the helper opens the relative path php/etc/php.ini. createExtensionContext does call ctx.Set("BUILD_DIR", s.Stager.BuildDir()), but Set writes into the Data map and does not reach the struct field. Every other extension reads ctx.GetString("BUILD_DIR") and is unaffected, which is why only the sessions path breaks.

The build directory is also the wrong base. installPHP uses filepath.Join(s.Stager.DepDir(), "php"), and php.ini is written to <deps>/<idx>/php/etc/php.ini. Nothing writes to <build>/php.

Fix

Resolve php.ini and php-fpm.conf from ctx.GetString("DEPS_DIR"), which the supply phase sets to s.Stager.DepDir().

Test

sessions_test.go passed only because it assigned ctx.BuildDir directly, with a comment noting that Set() would not work, and created php/etc under the build directory. That layout does not exist at staging time, so the test could not catch this. It now models the dependency directory. All unit tests pass; src/php/brats fails identically before and after this change, on a DeferCleanup misuse in the vendored bratshelper.

@ocean90
Read php.ini from the dependency directory in the sessions extension
2f6aa04 
The sessions extension resolves php.ini through PHPConfigHelper, which
builds its paths from ctx.BuildDir. That field is populated in NewContext
from a BUILD_DIR environment variable that the lifecycle never exports, so
it is empty and the helper opens the relative path php/etc/php.ini. Any
app with a bound Redis or Memcached session service therefore fails
staging with:

  Extension compilation failed: extension sessions compile failed: failed
  to load PHP config: failed to load php.ini: failed to read config file
  php/etc/php.ini

The supply phase records the real location under the DEPS_DIR key, and
that is also where PHP is installed: php.ini lives in <deps>/<idx>/php/etc,
never under the build directory.

The unit test passed only because it assigned ctx.BuildDir directly,
bypassing the code path production uses, and laid out php/etc under the
build directory. It now models the dependency directory instead.
@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 29, 2026 •
edited
Loading

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: ocean90 / name: Dominik Schilling (2f6aa04)

@benjaminguttmann-avtq

benjaminguttmann-avtq commented Oct 21, 2026

Copy link
Copy Markdown

Asked my AI to review this PR

What the change does

NewPHPConfigHelper built php.ini and php-fpm.conf paths from ctx.BuildDir. That field comes from a
BUILD_DIR environment variable the lifecycle never exports, so it is empty and the helper opened the
relative path php/etc/php.ini. Any app with a Redis or Memcached session service failed staging.
The change reads ctx.GetString("DEPS_DIR") instead.

Verification of the fix

  • supply.go L180 sets DEPS_DIR from Stager.DepDir(), which is depsDir/depsIdx.
  • PHP and its config are installed to DepDir()/php and DepDir()/php/etc (InstallPHP, L372 onward).
  • Extension Compile runs after InstallPHP, so the file exists when the sessions extension reads it.
  • The sessions extension is the only caller of NewPHPConfigHelper.

Conclusion: the fix is correct and minimal.

Findings

[LOW] src/php/extensions/extension.go L450-458 — error handling

  • If DEPS_DIR is not set, filepath.Join("", "php", "etc") gives the relative path php/etc/php.ini.
    The error is then the same confusing message as in the original bug report.
  • Fix: return an explicit error such as DEPS_DIR not set in extension context from NewPHPConfigHelper
    or LoadConfig when the value is empty.

[LOW] src/php/extensions/extension.go L87 and L450 — leftover Context.BuildDir

  • Context.BuildDir and Context.DepsDir are filled from environment variables that are not exported.
    They stay empty, so the struct offers two plausible sources and one is always empty.
  • Fix: remove the fields, or fill them from the Data keys and keep one source.

[LOW] src/php/extensions/sessions/sessions_test.go L22-31 — test wiring

  • The test sets DEPS_DIR directly with ctx.Set, not through createExtensionContext.
    A regression in the wiring at supply.go L180 would not be caught. This is the same blind spot the
    commit message criticises in the old test.
  • Fix: add a test in supply_test.go asserting ctx.GetString("DEPS_DIR") == Stager.DepDir().

[LOW] Branch base

  • The diff against current origin/master is dominated by vendored code because the branch is based on an
    older state. Rebase before an upstream PR.

Follow-up check: composer and New Relic

The original review raised a MEDIUM finding that composer and newrelic still use BUILD_DIR/php/....
Result of the check (details in php-buildpack-review-sessions-php-ini-path.md):

  • Composer: not a bug. It installs its own PHP into BUILD_DIR/php and configures it there.
  • New Relic: likely broken or inactive, independent of this commit. Configure reads
    BUILD_DIR/php/etc/php.ini before InstallPHP, edits a php.ini that is not the one used at runtime,
    and is gated by PHP_VM, which only the composer extension sets. Confidence is medium and needs a
    staging run to confirm. Track as a separate issue, not in this PR.

@benjaminguttmann-avtq

benjaminguttmann-avtq commented Oct 21, 2026

Copy link
Copy Markdown

So I would say there is nothing blocking it from getting merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@benjaminguttmann-avtq benjaminguttmann-avtq benjaminguttmann-avtq approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ocean90 @benjaminguttmann-avtq